-
Notifications
You must be signed in to change notification settings - Fork 0
[SSF-102] Add endpoints to update user info #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
dburkhart07
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small changes. Additionally, can we use this PR to update the user entity? If you notice, all the other fieldsin our entity have a name, type, and sometimes a few other things. Can we apply that to firstName, lastName, and email?
|
|
||
| export class updateUserInfo { | ||
| @IsOptional() | ||
| @IsString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add @IsNotEmpty and @MaxLength(255) to this and the last name?
| lastName?: string; | ||
|
|
||
| @IsOptional() | ||
| @IsString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add @IsNotEmpty here too?
| async updateInfo( | ||
| @Param('id', ParseIntPipe) id: number, | ||
| @Body() updateUserInfo: updateUserInfo, | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we give this a Promise<User> return type?
|
|
||
| describe('PUT :id/info', () => { | ||
| it('should update user info with valid information', async () => { | ||
| const updatedUser = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we type this? I would suggest moving the updateUserSchema to lin 122, and then making the value for each in the updatedUser `updateUserSchema.{firstName, lastName, phone}
|
|
||
| expect(result).toEqual(mockUser1); | ||
| expect(mockUserService.update).toHaveBeenCalledWith(1, updateUserSchema); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add one more test here where we just update with the first name and last name, to test that an actual Partial<updateUserInfo> works? I only ask this since our logic for creating the body we pass into the service is in the controller.
ℹ️ Issue
Closes 102
📝 Description
Added an endpoint to update a user’s first name, last name, or phone number and tests for the controller to verify that it works.
✔️ Verification
Tested having no parameters, 2 parameters and 3 parameters and throwing the proper exceptions when having faulty data.
